changing ANTS input traits, fixes #2245 #2261

djarecka · 2017-10-29T16:08:21Z

I've changed metric_weight (took default from other interfaces) and radius (left without default value).

Unfortunately other interfaces from that file have similar problems (do not work after providing all mandatory fields) and it was already reported in #529.
I can keep adding mandatory to traits and guessing default values, but it might be better for those interfaces if someone who is using them do it (or help me).

codecov-io · 2017-10-29T18:04:03Z

Codecov Report

Merging #2261 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2261      +/-   ##
==========================================
- Coverage   72.37%   72.32%   -0.06%     
==========================================
  Files        1188     1183       -5     
  Lines       59184    59008     -176     
  Branches     8506     8490      -16     
==========================================
- Hits        42837    42675     -162     
+ Misses      14961    14949      -12     
+ Partials     1386     1384       -2

Flag	Coverage Δ
#smoketests	`72.32% <100%> (-0.06%)`	⬇️
#unittests	`69.89% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/ants/registration.py	`72.86% <100%> (ø)`	⬆️
nipype/interfaces/ants/resampling.py	`80.45% <0%> (-3.42%)`	⬇️
nipype/utils/profiler.py	`40.36% <0%> (-1.21%)`	⬇️
nipype/interfaces/tests/test_resource_monitor.py	`60% <0%> (-0.98%)`	⬇️
nipype/interfaces/afni/preprocess.py	`80.32% <0%> (-0.2%)`	⬇️
nipype/pipeline/engine/workflows.py	`78.78% <0%> (-0.04%)`	⬇️
nipype/pipeline/engine/utils.py	`79.42% <0%> (-0.03%)`	⬇️
nipype/interfaces/bids_utils.py	`81.03% <0%> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_Warp.py	`85.71% <0%> (ø)`	⬆️
nipype/utils/tests/test_misc.py	`100% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2c5b4...ec5d3b1. Read the comment docs.

codecov-io · 2017-10-29T18:04:08Z

Codecov Report

Merging #2261 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2261      +/-   ##
==========================================
- Coverage   72.37%   72.32%   -0.06%     
==========================================
  Files        1188     1183       -5     
  Lines       59184    59008     -176     
  Branches     8506     8490      -16     
==========================================
- Hits        42837    42675     -162     
+ Misses      14961    14949      -12     
+ Partials     1386     1384       -2

Flag	Coverage Δ
#smoketests	`72.32% <100%> (-0.06%)`	⬇️
#unittests	`69.89% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/ants/registration.py	`72.86% <100%> (ø)`	⬆️
nipype/interfaces/ants/resampling.py	`80.45% <0%> (-3.42%)`	⬇️
nipype/utils/profiler.py	`40.36% <0%> (-1.21%)`	⬇️
nipype/interfaces/tests/test_resource_monitor.py	`60% <0%> (-0.98%)`	⬇️
nipype/interfaces/afni/preprocess.py	`80.32% <0%> (-0.2%)`	⬇️
nipype/pipeline/engine/workflows.py	`78.78% <0%> (-0.04%)`	⬇️
nipype/pipeline/engine/utils.py	`79.42% <0%> (-0.03%)`	⬇️
nipype/interfaces/bids_utils.py	`81.03% <0%> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_Qwarp.py	`85.71% <0%> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_Warp.py	`85.71% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2c5b4...ec5d3b1. Read the comment docs.

codecov-io · 2017-10-29T18:04:18Z

Codecov Report

Merging #2261 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2261      +/-   ##
==========================================
- Coverage   72.37%   72.32%   -0.06%     
==========================================
  Files        1188     1184       -4     
  Lines       59184    59024     -160     
  Branches     8506     8490      -16     
==========================================
- Hits        42837    42689     -148     
+ Misses      14961    14953       -8     
+ Partials     1386     1382       -4

Flag	Coverage Δ
#smoketests	`72.32% <100%> (-0.06%)`	⬇️
#unittests	`69.9% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/ants/registration.py	`72.86% <100%> (ø)`	⬆️
nipype/interfaces/ants/tests/test_registration.py	`100% <100%> (ø)`
nipype/interfaces/dipy/setup.py	`0% <0%> (-25%)`	⬇️
nipype/interfaces/setup.py	`0% <0%> (-6.46%)`	⬇️
nipype/interfaces/ants/resampling.py	`80.45% <0%> (-3.42%)`	⬇️
nipype/interfaces/tests/test_resource_monitor.py	`60% <0%> (-0.98%)`	⬇️
nipype/interfaces/afni/preprocess.py	`80.32% <0%> (-0.2%)`	⬇️
nipype/pipeline/engine/workflows.py	`78.78% <0%> (-0.04%)`	⬇️
nipype/pipeline/engine/utils.py	`79.42% <0%> (-0.03%)`	⬇️
nipype/interfaces/bids_utils.py	`81.03% <0%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2c5b4...51585be. Read the comment docs.

satra · 2017-10-30T15:20:57Z

@djarecka - can we use this example to add a test to regression testing framework? i feel that's what we need to do from here on out. interfaces like antsRegistration have too many options to generate all kinds of tests automatically. but we can use a combination of manual examples that actually test the binary. this obviously won't be part of the pull-request, but it would be nice to use this example as a test case in the testing framework.

satra · 2017-10-30T15:21:26Z

nipype/interfaces/ants/registration.py

+                                desc='the metric weight(s) for each stage. '
+                                'The weights must sum to 1 per stage.')
+
+    radius = traits.List(traits.Int(), requires=['metric'],  mandatory=True, desc='')


no description added.

I added some description, but originally none of the traits had any description, and most of the still don't have. I can write the description when working on tests. I guess we can keep this PR open for some time

There is any typical value that could be used as default?

djarecka · 2017-10-31T14:34:39Z

@satra : ok, will work on that

satra · 2017-10-31T14:56:27Z

@djarecka - a mandatory trait should not have a default value in general. we leave it to the user to provide it. but there are cases when this may be necessary. i don't think this is one of them.

descriptions should come from the actual command line interface.

djarecka · 2017-10-31T15:01:26Z

@satra - hmm... i used default for metric_weight, because i found that other interfaces in the file use this syntax.

satra · 2017-11-12T16:26:09Z

@djarecka - is this ready? or there other changes coming?

djarecka · 2017-11-12T19:03:24Z

@satra it fixes the issue (and just added a simple example that I was using to test it. However this entire file requires review and tests. Other interfaces have very similar problems.

We can either keep this open or open a new one later. If no ANTS user wants to review it I can try to compare with ANTS documentation

mgxd

haven't tested it out but LGTM

mgxd · 2017-11-27T19:06:31Z

nipype/interfaces/ants/tests/test_registration.py

+    ants.inputs.transformation_model= "SyN"
+    ants.inputs.moving_image = [os.path.join(datadir, 'resting.nii')]
+    ants.inputs.fixed_image = [os.path.join(datadir, 'T1.nii')]
+    ants.inputs.metric = [u'MI']


can we just import unicode literals?

oesteban

LGTM, couple of minor nitpicks

oesteban · 2017-11-27T22:15:20Z

nipype/interfaces/ants/registration.py

+    metric_weight = traits.List(traits.Float(), value=[1.0], usedefault=True,
+                                requires=['metric'], mandatory=True,
+                                desc='the metric weight(s) for each stage. '
+                                'The weights must sum to 1 per stage.')


isn't this a pep8 warning?

this line doesn't give any warning, but the files indeed violates many. will edit

I will actually review pep8 only for the first part I was changing, will review pep8 for entire file together with other updates

oesteban · 2017-11-27T22:17:50Z

nipype/interfaces/ants/tests/test_registration.py

+import os
+import pytest
+
+def test_ants_mand():


does the interface actually create files? if so (and I guess it would), we should include tmpdir.chdir()

changing some ants inputs to mandatory

ec5d3b1

satra added this to the 0.14.0 milestone Oct 29, 2017

satra reviewed Oct 30, 2017

View reviewed changes

adding description to radius

3196726

adding a simple example to test issue 2245

a1d62ca

djarecka mentioned this pull request Nov 26, 2017

ANTS.registration requires reviews #2305

Open

mgxd approved these changes Nov 27, 2017

View reviewed changes

unicode_literals

e214b54

oesteban approved these changes Nov 27, 2017

View reviewed changes

ading tmpdir; applying pep8 to my changes

51585be

mgxd merged commit f1d690c into nipy:master Nov 28, 2017

changing ANTS input traits, fixes #2245 #2261

changing ANTS input traits, fixes #2245 #2261

Uh oh!

Conversation

djarecka commented Oct 29, 2017

Uh oh!

codecov-io commented Oct 29, 2017

Codecov Report

Uh oh!

codecov-io commented Oct 29, 2017

Codecov Report

Uh oh!

codecov-io commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

satra commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djarecka commented Oct 31, 2017

Uh oh!

satra commented Oct 31, 2017

Uh oh!

djarecka commented Oct 31, 2017

Uh oh!

satra commented Nov 12, 2017

Uh oh!

djarecka commented Nov 12, 2017

Uh oh!

mgxd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djarecka Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 29, 2017 •

edited

Loading

djarecka Nov 27, 2017 •

edited

Loading